-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DM-45906: Create app that can use Butler to implement IVOA SIAv2 protocol #3
Conversation
afdf067
to
a187db8
Compare
7327e38
to
abc1abc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an absolute ton of work and overall looks great! Most of the comments are small things. I just have a few larger structural things.
I vote for dropping local Butler support. Our position has been that Science Platform services will only support remote Butler, and I've dropped local Butler support from vo-cutouts and datalinker already. I'm worried that someone will use local Butler support and then be upset when we delete it later.
Updating the Makefile
to the latest from the FastAPI template, that will decide whether to generate hashes per dependency and you can stop thinking about that.
I've applied hopefully all the suggested changes. There are a few more changes (sorry about the quite long commit) which are related to:
|
714c182
to
6361268
Compare
b9215aa
to
f642174
Compare
I've left the requirements as non-hashed because dax_obscore isn't on pypi yet. Hopefully it is soon and I can update the requirements to included the hashes, do we want to wait for that before merging? Is there a better way I should handle this? |
3c8166e
to
a66354c
Compare
…y and fix mobu typo
9d65929
to
fd086c2
Compare
Note that I've in parallel made some changes that I'll wait for PR after this is finished, which introduces a self-description VOTable document & also adds a dependency which loads the obscore configs during the app initialization, so that there are no calls to the external config file during the query. I didn't want to include them in this PR as to not introduce additional changes on top of the recent PR fixes. |
b8853b0
to
28197e4
Compare
You should be able to use the latest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new middleware looks great! I am really excited if this works, since this will solve a bunch of problems for IVOA protocol apps.
This all looks great. I left a few more comments on the unresolved conversations, but overall I'm great with merging this all. This looks very solid.
8ce125b
to
4f80641
Compare
… and add documentation for helper endpoints
9187c12
to
798bc38
Compare
Yep that worked, just had to modify the Dockerfile to get it to build without complaining about not having hashes for that github repo |
Summary
PR introduces the SIA v2 (over Butler) FastAPI application. The app utilizes the dax_obscore & daf_butler packages to search for images that match certain query parameters. The API follows the IVOA specification: https://www.ivoa.net/documents/SIA/
A few things to note:
Design decisions
Current system was designed to support Direct & Remote butler configurations. There is a factory method create_butler which should create the butler based on the configuration passed in. Initially the designed was more abstract, where I defined the notion of a
QueryEngine
, andDirectButlerEngine
,RemoteButlerEngine
were implementations, however this felt too abstract and an early optimization for something that may not be needed, i.e. other implementations ofsiav2_query
.The main configuration other than the
butler_type
needed isbutler_data_collections
. This expects a list of ButlerDataCollection objects, which has following parameters, all of which are used to generate the right Butler object. Reason for butler_data_collections being a list, is that we may in the future want to support multiple different Butler repos.The query parameters are defined as a dataclass, because it seemed difficult to get a Pydantic model working with list query parameters (related issue: fastapi/fastapi#10556). Also siav2_query has it's own Pydantic model for the SIAv2 Parameters with it's own validation, it seemed better to avoid duplicating the validation.
Prototype notebook
Example notebook demonstrating access via python can be found here:
https://github.com/lsst-sqre/sia/blob/u/stvoutsin/example-notebook/examples/SIAv2_Example.ipynb
How was this tested
A test instance was deployed on data-dev. Process was to modify science-platform app to point to phalanx where sia is enabled, sync only sia and then point the synced sia app to the GH dev branch.
Sample query:
https://data-dev.lsst.cloud/api/sia/query?POS=CIRCLE+55.7467+-32.2862+0.05&time=60550.31803461111+60550.31838182871
Validation was run (using a sample HSC direct butler setup) with the following services and passed with an A++ rating
Euro-vo validation
Paris-vo validation
What hasn't been tested
Apart from the above query, I haven't tested many other parameters, as I'm not really sure about the holdings and what type of queries should return data.
I also haven't fully concluded any concurrent access tests (Ongoing work).